-
Notifications
You must be signed in to change notification settings - Fork 222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
adding correct annotations for svcms and fixing sa #439
base: main
Are you sure you want to change the base?
adding correct annotations for svcms and fixing sa #439
Conversation
a70bb2c
to
e6aa99c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some breaking changes, but would you mind also fixing DCO?
Signed-off-by: Tom Kennes <[email protected]>
Signed-off-by: Tom Kennes <[email protected]>
e6aa99c
to
285a67d
Compare
Signed-off-by: Tom Kennes <[email protected]>
@tomkerkhove : I don't really understand the lack of flexible when it comes to setting annotations for specific resources, could you explain? Also, would it not make more sense to opt for some sort of global + local mechanism to set labels? See also here for example: https://github.com/argoproj/argo-helm/blob/main/charts/argo-cd/templates/argocd-application-controller/statefulset.yaml#L3 |
Can you elaborate on this please? My comments were only to keep supporting what we already support, I'm find adding more annotations if there is a need, as long as it does not break things =) |
@tomkerkhove , in that case, it depends a bit on the cluster whether annotations are heavily used for individual resources. As a rule of thumb, you would use labels for most things especially when it comes to managing infrastructure and tying certain resources together. E.g. a service binds to a pod based on the label, same for servicemonitors and podmonitors. This is also why you cannot really use labels if you intend to segment resources further, as you will have to be careful not to misuse any labels. Hence, you use annotations. I have seen companies use annotations to route alerts to the right teams for example. Therefore, it might make sense to allow for individual annotations per resource rather than having only one global annotation. If you see benefit in the approach, I can set this up for all resources like for ArgoCD (https://github.com/argoproj/argo-helm/blob/main/charts/argo-cd/templates/argocd-application-controller/statefulset.yaml#L3), e.g., we merge global .Values.additionalAnnotations with local resource specific ones, like for example .Values.prometheus.metricServer.serviceMonitor.additionalAnnotations. |
Signed-off-by: Tom Kennes <[email protected]>
55727c5
to
604bc7e
Compare
@tomkerkhove , this should be resolved by now right? |
This is OK for me, can you pull in the changes from main & document the new options please? |
I don't think you need documentation here, since you are not documenting the other fields of these service- and podmonitors and since they are basic fields. This approach is also more common, instead of setting annotations over all resoruces using one global value. Let me know if you disagree, I can add a comment in the values.yaml if you really insist, or let me know where you want these comments. |
We should document all options, even if they are well-known ones because people who are new to the cloud-native space would not know about them unfortunately. Given #444 is being done, it's OK to just document them in the values file, thanks! |
I disagree on this one, but sure, there you go |
Signed-off-by: Tom Kennes <[email protected]>
6d435e3
to
7c43d45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Would you mind fixing the merge conflicts please?
Provide a description of what has been changed
Fixes #437 (comment)